Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Personal stats #18

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

WillLillis
Copy link
Contributor

As suggested in CONTRIBUTING.md, this feature adds the ability to display a user's personal stats for a given year.

While testing, I noticed that if you try to access personal stats while not logged in, the site redirects to show the global stats. I currently have show_personal_stats() return an AocError::AocResponseError in this case, but I wasn't super sure if that correctly describes what's happening. Any suggestions for a different error to return when this happens?

Comment on lines 437 to 444
} else if response.status() == StatusCode::FOUND {
// A 302 reponse is a redirect and it likely
// means we're not logged in
warn!(
"🍪 It looks like you are not logged in, try logging in again"
);
return Err(AocError::AocResponseError);
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this deserves a new error, say AocError::NotLoggedIn. When showing the calendar not being logged in isn't a problem as you can still show the calendar with no stars. But here it's different...

Comment on lines 471 to 476
"--------Part 1---------",
&"--------Part 1---------".color(SILVER).to_string(),
)
.replace(
"--------Part 2---------",
&"--------Part 2---------".color(GOLD).to_string(),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the size of these headers can vary, and they will not be correctly coloured if they're not exactly the same size as in these strings

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad! Maybe a regex would be a better fit here?

Comment on lines 482 to 483
.replacen("Rank", &"Rank".color(SILVER).to_string(), 2) // "Rank" also in explanation
.replacen("Score", &"Score".color(SILVER).to_string(), 2); // "Score" also in
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The words in the header are not being coloured as you printed them before (on lines 465-466). We can probably leave the explanation uncoloured though

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for being unclear in the comments, it was my intention for the explanation to remain uncolored since that is the behavior displayed on the website.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants